-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add HMAC/IP support for different remote IP's through proxies & Cloud… #49
base: master
Are you sure you want to change the base?
Conversation
if (isset($this->server["HTTP_X_FORWARDED_FOR"]) && filter_var($this->server["HTTP_X_FORWARDED_FOR"], FILTER_VALIDATE_IP)) | ||
{ | ||
return $this->server["HTTP_X_FORWARDED_FOR"]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attackers can lie about X-Forwarded-For
. This should be guarded by a configuration setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you suggest? A configuration setting as in hmac_ip_allow_x_forwarded_for? Or something more general that would include both this option as well as CloudFlare for example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a configuration setting would be a good idea. Let people opt into this behavior.
{ | ||
if (isset($this->server["HTTP_CF_CONNECTING_IP"]) && filter_var($this->server["HTTP_CF_CONNECTING_IP"], FILTER_VALIDATE_IP)) | ||
{ | ||
return $this->server["HTTP_CF_CONNECTING_IP"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if an attacker specifies a fake header here after discovering the server's real IP?
What if an attacker specifies this header for a non-CloudFlare IP?
Etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you suggest validating against the REMOTE_ADDR to the CloudFlare IP ranges? https://www.cloudflare.com/ips/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and this can be hard-coded. We can update it in the future if their list of IPs widens.
@paragonie-security I've thought about your suggestion and played around a bit but I think there's several approaches that I wanted to check with you first. Especially for the IP filtering for CloudFlare, there's multiple options:
or
in that case there's also two options:
Could you share with me your preferences? |
The library's default is to have the HMAC IP check enabled. This gives issues with sites that run through CloudFlare as the remote IP that CloudFlare connects from is different through requests. Thus this could result in an invalid request already if you have a page open for 2 minutes.
The added functionality adds support for the CloudFlare connecting IP and other commonly used proxy methods. With these changes you can continue to use the HMAC check through CloudFlare/proxies.